Update blackbox tests to use rules_pkg instead of built-ins for pkg_tar #9046
Conversation
We add rules_pkg to the default WORKSPACE and then try to use that. Alas, things fail because starlark is getting rid of the outputs attribute, and the test runs with the incompatible changes on. A better solution may be to figure out why we need tar for the repo tests. If we can eliminate that, a lot of problems go away.
aiuto
added
team-ExternalDeps
|
Sorry, I won't have time for this, happy to review a working PR, but can't do anything else :/ |
//src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace:GitRepositoryBlackBoxTest FAILED in 50.5s //src/test/java/com/google/devtools/build/lib/blackbox/tests/workspace:WorkspaceBlackBoxTest FAILED in 68.7s The entire test scheme is wonky, in that it depends on pkg_tar in the first place. Something is going wrong with the indirect loading of the rules via RepoWithRuleWritingTextGenerator
|
I saw this error in your tests: We run tests without network access by default now and override repositories used by integration tests to be the same as the repos from the "outer" Bazel. Relevant changes that explain what's going on here: You probably just have to add Also we have to make sure that the version of rules_pkg in Bazel's WORKSPACE file and the one specified in the test's WORKSPACE file is the same. So we should bump rules_pkg in Bazel's WORKSPACE also to 0.2.5 and add a comment where to keep the two in sync. |
|
Thanks. I missed that one.
That is a nice to have, but not actually needed. Since we only use pkg_tar to package bazel, and for anything else, this integration test could use any version of it. That points to some of the strangeness here. It makes a tarball, but then never uses it. I have mail out to the original author to confirm my suspicion that pkg_tar was just a convenient way to have a rule that writes content based on another rule and have it done at workspace time. |
|
Damm. Broken possibly because of #9029. Need to come back to this next month. |
aiuto
changed the title
Oh. My bad. I didn't read your comment about forcing repositories to be the outer ones. So yes. I do have to update all of Bazel. PR #11191 will do that first. |
philwo
added
the
team-OSS
|
I'm rethinking this. These tests should not have to depend on @rules_pkg. They should be rewritten to be self contained. |
|
This PR won't work any more. |
philwo
removed
the
team-OSS
We add rules_pkg to the generated WORKSPACE for integration tests.
A better solution may be to figure out why we need tar for the repo tests. If we can eliminate that, a lot of problems go away., but that job belongs to the external deps team.
WIP: Alas, things fail because starlark is getting rid of the outputs attribute, and the test runs with the incompatible changes on. We have to fix that in rules_pkg and use version 0.2.1 of that
WIP: This solution is super brittle because we have to repeat repo urls and sha256 identically in many places. #8986. Since I have a few weeks before I have to do this, perhaps we can come up with a better design for #8986 now and work towards that. @philwo: do you want me to propose one or do you want to take a stab at it.